-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Reduce generics use in the query system. #151777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
|
Yea this makes sense. Not much of an improvement, but lifetime generics are better than type generics for readability |
|
Be aware that this will textually conflict with some of the renames in #151666, though the changes should be compatible. Also, after that PR I intend to change the generic type of |
This comment has been minimized.
This comment has been minimized.
|
The original implementation tried to avoid adding the |
I'll look into this as a follow-up. I would definitely like to get rid of the unsafe transmute. |
Make `QueryDispatcher::Qcx` an associated type There's no reason to think that a query dispatcher/vtable would support multiple query-context types, and this simplifies some generic signature boilerplate. --- - This is the planned change that was mentioned in rust-lang#151777 (comment). r? nnethercote
| /// A type representing the responsibility to execute the job in the `job` field. | ||
| /// This will poison the relevant query if dropped. | ||
| struct JobOwner<'tcx, K, I> | ||
| struct JobOwner<'a, 'tcx, K> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come 'a snuck in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It can be removed, but requires a bunch of follow-on lifetime changes, shown below. I think having the 'a is preferable -- an extra lifetime contained within JobOwner is better than extra lifetime bounds spread over multiple functions and files.
diff --git a/compiler/rustc_query_impl/src/plumbing.rs b/compiler/rustc_query_impl/src/plumbing.rs
index ef7a6235193..d9b1f5a3716 100644
--- a/compiler/rustc_query_impl/src/plumbing.rs
+++ b/compiler/rustc_query_impl/src/plumbing.rs
@@ -484,7 +484,7 @@ pub(crate) fn try_load_from_disk<'tcx, V>(
fn force_from_dep_node<'tcx, Q>(query: Q, tcx: TyCtxt<'tcx>, dep_node: DepNode) -> bool
where
- Q: QueryDispatcher<'tcx, Qcx = QueryCtxt<'tcx>>,
+ Q: QueryDispatcher<'tcx, Qcx = QueryCtxt<'tcx>> + 'tcx,
{
// We must avoid ever having to call `force_from_dep_node()` for a
// `DepNode::codegen_unit`:
@@ -517,7 +517,7 @@ pub(crate) fn make_dep_kind_vtable_for_query<'tcx, Q>(
is_eval_always: bool,
) -> DepKindVTable<'tcx>
where
- Q: QueryDispatcherUnerased<'tcx>,
+ Q: QueryDispatcherUnerased<'tcx> + 'tcx,
{
let fingerprint_style = <Q::Dispatcher as QueryDispatcher>::Key::fingerprint_style();
diff --git a/compiler/rustc_query_system/src/query/plumbing.rs b/compiler/rustc_query_system/src/query/plumbing.rs
index 6cd259f4d3b..51ea938fb1f 100644
--- a/compiler/rustc_query_system/src/query/plumbing.rs
+++ b/compiler/rustc_query_system/src/query/plumbing.rs
@@ -117,11 +117,11 @@ fn default() -> QueryState<'tcx, K> {
/// A type representing the responsibility to execute the job in the `job` field.
/// This will poison the relevant query if dropped.
-struct JobOwner<'a, 'tcx, K>
+struct JobOwner<'tcx, K>
where
K: Eq + Hash + Copy,
{
- state: &'a QueryState<'tcx, K>,
+ state: &'tcx QueryState<'tcx, K>,
key: K,
}
@@ -171,7 +171,7 @@ fn handle_cycle_error<'tcx, Q>(
}
}
-impl<'a, 'tcx, K> JobOwner<'a, 'tcx, K>
+impl<'tcx, K> JobOwner<'tcx, K>
where
K: Eq + Hash + Copy,
{
@@ -208,7 +208,7 @@ fn complete<C>(self, cache: &C, key_hash: u64, result: C::Value, dep_node_index: }
}
-impl<'a, 'tcx, K> Drop for JobOwner<'a, 'tcx, K>
+impl<'tcx, K> Drop for JobOwner<'tcx, K>
where
K: Eq + Hash + Copy,
{
@@ -348,7 +348,7 @@ fn try_execute_query<'tcx, Q, const INCR: bool>(
dep_node: Option<DepNode>,
) -> (Q::Value, Option<DepNodeIndex>)
where
- Q: QueryDispatcher<'tcx>,
+ Q: QueryDispatcher<'tcx> + 'tcx,
{
let state = query.query_state(qcx);
let key_hash = sharded::make_hash(&key);
@@ -412,7 +412,7 @@ fn try_execute_query<'tcx, Q, const INCR: bool>(
fn execute_job<'tcx, Q, const INCR: bool>(
query: Q,
qcx: Q::Qcx,
- state: &QueryState<'tcx, Q::Key>,
+ state: &'tcx QueryState<'tcx, Q::Key>,
key: Q::Key,
key_hash: u64,
id: QueryJobId,
@@ -812,7 +812,7 @@ pub enum QueryMode {
#[inline(always)]
pub fn get_query_non_incr<'tcx, Q>(query: Q, qcx: Q::Qcx, span: Span, key: Q::Key) -> Q::Value
where
- Q: QueryDispatcher<'tcx>,
+ Q: QueryDispatcher<'tcx> + 'tcx,
{
debug_assert!(!qcx.dep_context().dep_graph().is_fully_enabled());
@@ -828,7 +828,7 @@ pub fn get_query_incr<'tcx, Q>(
mode: QueryMode,
) -> Option<Q::Value>
where
- Q: QueryDispatcher<'tcx>,
+ Q: QueryDispatcher<'tcx> + 'tcx,
{
debug_assert!(qcx.dep_context().dep_graph().is_fully_enabled());
@@ -852,7 +852,7 @@ pub fn get_query_incr<'tcx, Q>(
pub fn force_query<'tcx, Q>(query: Q, qcx: Q::Qcx, key: Q::Key, dep_node: DepNode)
where
- Q: QueryDispatcher<'tcx>,
+ Q: QueryDispatcher<'tcx> + 'tcx,
{
// We may be concurrently trying both execute and force a query.
// Ensure that only one of them runs the query.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking QueryDispatcher::query_state could return &'tcx QueryState now that it has access to 'tcx. That might help with the lifetime bounds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that JobOwner is a helper type that is only used locally, giving it the burden of an extra 'a does indeed seem preferable to larger changes, at least for this PR.
Make `QueryDispatcher::Qcx` an associated type There's no reason to think that a query dispatcher/vtable would support multiple query-context types, and this simplifies some generic signature boilerplate. --- - This is the planned change that was mentioned in rust-lang#151777 (comment). r? nnethercote
Rollup merge of #151802 - Zalathar:qcx, r=nnethercote Make `QueryDispatcher::Qcx` an associated type There's no reason to think that a query dispatcher/vtable would support multiple query-context types, and this simplifies some generic signature boilerplate. --- - This is the planned change that was mentioned in #151777 (comment). r? nnethercote
Instead of passing a closure that computes a hash and immediately calling it, just compute the hash and pass the value.
It produces a `QueryStackFrameExtra`, so `stack_` should be in the name.
`QueryStackFrame<I>` is a generic type, and the `I` parameter leaks out to many other related types. However, it only has two instantiations in practice: - `QueryStackFrame<QueryStackFrameExtra>` - `QueryStackFrame<QueryStackDeferred<'tcx>>` And most of the places that currently use `QueryStackFrame<I>` are actually specific to one of the instantiations. This commit removes the unneeded genericity. The following types are only ever used with `QueryStackDeferred<'tcx>`: - QueryMap - QueryJobInfo - QueryJob - QueryWaiter - QueryLatchInfo - QueryLatch - QueryState - QueryResult and their `<I>` parameter is changed to `<'tcx>`. Also, the `QueryContext::QueryInfo` associated type is removed. `CycleError<I>` and `QueryInfo<I>` are still generic over type, because they are used with both instantiations. This required also adding a `<'tcx>` parameter to the traits `QueryDispatcher` and `QueryContext`, which is annoying but I can't see how to avoid it.
2afcbf6 to
1c3d9ab
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ oli-obk |
…ercote
Reduce generics use in the query system.
In rust-lang#151203 I tried and failed to simplify `QueryStackFrame`. This is an alternative approach. There is no functional change, just tweaking of some names and types to make things clearer. Best reviewed one commit at a time.
r? @oli-obk
In #151203 I tried and failed to simplify
QueryStackFrame. This is an alternative approach. There is no functional change, just tweaking of some names and types to make things clearer. Best reviewed one commit at a time.r? @oli-obk